Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(group): add message expiration to group metadata #1477

Merged
merged 12 commits into from
Jan 10, 2025

Conversation

mchenani
Copy link
Contributor

@mchenani mchenani commented Jan 7, 2025

This pull request adds support for message expiration policies in group settings. The key updates include:
• A new message_expiration_ms field added to group creation options: holds how long after the message was received gets deleted
• A new message_expiration_from_ms field added to group creation options: holds from when the messages get deleted
• Permission updates to allow managing message expiration settings.

#1449

@mchenani mchenani marked this pull request as ready for review January 7, 2025 17:15
@mchenani mchenani requested a review from a team as a code owner January 7, 2025 17:15
@@ -55,6 +55,7 @@ pub const DEFAULT_GROUP_NAME: &str = "";
pub const DEFAULT_GROUP_DESCRIPTION: &str = "";
pub const DEFAULT_GROUP_IMAGE_URL_SQUARE: &str = "";
pub const DEFAULT_GROUP_PINNED_FRAME_URL: &str = "";
pub const DEFAULT_MESSAGE_EXPIRATION_MS: i64 = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still thinking of setting this to null instead of 0, if you have another idea, lmk.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would. And just wrap the insert logic down below with a

if let Some(message_expiration_ms) = opts.message_expiration_ms {
  // insert...
}

Putting in a zero just seems like we're emulating null-state when we don't have to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cameronvoell might have thoughts on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since its a value in the map, instead of keeping a default value I decided to not insert it at all.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had issues in the past of if two people are using two different versions of the SDK in the same group chat. We need to make sure that the person on the old version of the sdk does not break. I believe there was some sort of migration that needs to happen but again @cameronvoell would remember better than me how we decided to go about this for new metadata.

Copy link
Contributor

@cameronvoell cameronvoell Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No migration needed here, existing groups will not have message expiration set, but updating the expiration set will be possible with default permission of admin_only (for groups created before this change goes in) =>

// Policy is not found for metadata change, let's check if the new field contains the super_admin prefix
// and evaluate accordingly
let policy_for_unrecognized_field =
if change.field_name.starts_with(SUPER_ADMIN_METADATA_PREFIX) {
MetadataPolicies::allow_if_actor_super_admin()
} else {
// Otherwise we default to admin only for fields with missing policies
MetadataPolicies::allow_if_actor_admin()
};

That path of adding new metadata to existing groups was tested with this change here, which we can reference for adding expiration in this pr.

@@ -161,12 +161,13 @@ pub struct PermissionPolicySet {
pub update_group_description_policy: PermissionPolicy,
pub update_group_image_url_square_policy: PermissionPolicy,
pub update_group_pinned_frame_url_policy: PermissionPolicy,
pub update_message_expiration_ms_policy: PermissionPolicy,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I Added only one policy for both message_expiration_ms and message_expiration_from_ms. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea I think that makes sense, doesnt make sense for someone to be able to change one without the other 👍

@mchenani mchenani merged commit 91e9f80 into main Jan 10, 2025
15 checks passed
@mchenani mchenani deleted the mc/add-message-expiration-to-group-metadata branch January 10, 2025 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants